feat(provider-tck): emit a machine-readable conformance report - #413
Draft
aepfli wants to merge 4 commits into
Draft
feat(provider-tck): emit a machine-readable conformance report#413aepfli wants to merge 4 commits into
aepfli wants to merge 4 commits into
Conversation
Setting PROVIDER_TCK_REPORT_DIR makes each suite write its run to <dir>/<name>.json against the report schema in the specification repository (open-feature/spec#425, part of open-feature/spec#424). Unset means no report, which is the default and is not an error. An environment variable rather than a TckConfig field, so that emitting a report is a property of the run and not of the code: CI sets it, a local run does not, and no adopter changes a line to publish one. Several suites in one pytest session each write their own file, so flagd's two resolvers would not collide. The load-bearing part is the per-scenario list. Appendix F requires that a scenario skipped for an undeclared capability is reported as skipped with the reason and never as passed, and nothing downstream can check that against a summary line. Recording every scenario's outcome individually makes the rule checkable by the consumer instead of dependent on the runner. It is also required to be complete, because a document that quietly dropped what it skipped would satisfy the letter of the rule and still mislead whoever read it. pytest, unlike godog, reports a skip honestly -- so the interesting divergence here is elsewhere. The one scenario the Python SDK cannot satisfy is marked xfail, so the run finishes green; the provider still did not satisfy it, and the document says failed with the reason. An expected failure is a recorded deviation, not an excused one. Scenarios are therefore enumerated at collection and resolved at the end of the session rather than as fixtures run, which is also what keeps a scenario skipped by a marker -- whose fixtures never run at all -- from vanishing from the document. Identity comes from spec_revision.json, generated by hatch_build_sync.py beside the copied assets and force-included into the wheel. It has to be captured at build time: the submodule that knows the answer is not in the distribution, so an installed copy has nothing left to ask. A build that cannot reach git -- an unpacked sdist -- warns and records "unknown" rather than inventing a commit. Both the commit and the tree hash are recorded, the tree because it identifies the assets alone: unchanged by unrelated edits elsewhere in the specification, so two runs of identical assets agree even when pinned to different commits, and checkable because `git rev-parse <commit>:specification/assets/provider-tck` reproduces it. Two smaller decisions. The provider is identified by the name it reports through its own metadata, with TckConfig.name recorded as the configuration, because TckConfig.name is chosen to read well in a failure message -- "flagd-rpc" -- and a provider with two materially different modes produces two reports that are not interchangeable. And how the backend was driven is read off an optional control_api property rather than added to the BackendControl protocol, so that adding it leaves every existing control complete and one that stays quiet simply omits the field. The tests assert the two properties a consumer is entitled to assume -- that no scenario the capability gate stopped is ever reported as passed, and that every collected scenario appears exactly once, counted against pytest's own collection rather than against a number written down beside it. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
A report entry was identified by feature and name. Every row of a Scenario Outline shares one name, so the eleven rows of the type-mismatch matrix in errors.feature produced eleven entries nothing could tell apart -- and in the Python run one of the eleven fails while ten pass, which is exactly the case the report could not express. A consumer keying on feature and name kept whichever row it happened to see last. Each entry from an outline now carries the row it came from, as the Examples parameters keyed by column header, matching the "example" property added to the schema. Values are the cell contents verbatim as strings: Gherkin has no types, so "1" stays "1" rather than becoming a number the table never mentioned. pytest-bdd parametrizes the generated test over one dict per row, keyed by the header, so the row is read back off the node's callspec -- available at collection, which is what lets a row the capability gate skipped be identified as precisely as one that ran. This removes the workaround that appended pytest's own id for the row to the scenario name. It was the wrong shape twice over. The name is the feature file's name, and qualifying it made Python disagree with Go and JavaScript about a scenario all three ran, which defeats the cross-language comparison the report exists for. And a name format would be normative text -- a separator, an ordering, an escaping rule -- that four languages have to reproduce byte for byte, where drift is invisible until two reports silently fail to line up. The parameters are the identity, and they come from the feature file rather than from any runner. The uniqueness test now keys on feature, name and example together, which is the property this change exists to establish. The examples the report emits are checked against the Examples tables read out of the Gherkin by hand, rather than against pytest-bdd's parser, which is what produced them. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
…ng untested ones
Two defects in the capability rollup, mirroring the fix already made in Go
(go-sdk-contrib#944).
A failed capability was emitted as {"state": "failed"} with no reason. The schema
now requires a reason for any outcome other than passed, so that entry does not
validate -- and it appears only when a provider is actually failing, which is
precisely when the report matters. It now says how many of how many scenarios
carrying the tag failed, and points at the per-scenario results for which and
why.
No test caught it because every self-test suite passes, so nothing that runs end
to end ever reaches that branch. The test now drives the report builder directly
with synthetic records, which is the only way to exercise a failure without
breaking a provider on purpose.
A declared capability that no scenario carries was reported as passed. @targeting
is reserved -- it exists in the vocabulary but nothing tests it, because asserting
that an evaluation context reached the backend needs an echo operation the
control API does not have -- so a provider declaring it got a green result for a
claim nothing had examined. That is the vacuous pass the capability vocabulary
was introduced to eliminate, arriving through the report rather than through the
suite.
Such a capability is now omitted. The suite asked no question, so it has no
answer to report, and a consumer sees the tag is absent rather than a pass it
cannot rely on. Omitting is preferred to inventing a fifth outcome: the four in
the schema are about what the provider did, and "the suite does not test this" is
a fact about the suite.
An undeclared capability is still reported with its reason whether or not any
scenario carries it, because that is a fact about the provider rather than about
the suite.
Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
Gherkin lets an Examples block carry its own tags, so two rows of one Scenario Outline can differ in which capability gates them. The capability gate already handled that correctly -- pytest-bdd attaches an Examples block's tags as marks on that block's parameter sets, and the gate reads the node's markers -- but the report did not. A scenario's tags were read from the scenario, the feature and the rule, which is everywhere those tags are not. The consequence was a misreport of exactly the kind the format exists to rule out. A row skipped because its Examples block was tagged with an undeclared capability appeared with no tags at all, so it was classified not-applicable rather than not-declared -- the run had a reason not to execute it, said the report, when the reason was a capability the provider does not have. The capability rollup did not count it either. The row's tags are now resolved by intersecting the tags the scenario's Examples blocks declare with the markers pytest put on the node. That names this row's blocks without having to work out which block a row came from, and admits nothing that is not a Gherkin tag of this scenario. No canonical feature file uses per-Examples tags today, so this is latent. It was found while checking a defect the Go implementation hit in the same area, where per-scenario bookkeeping keyed by scenario name let one gated row suppress the accounting for every row of its outline. Nothing here is keyed by name -- the collector, the durations and the records are all keyed by pytest node id, which is unique per row -- and the test added here confirms that every row of an outline is still reported when one of them is gated. Signed-off-by: Simon Schrottner <simon.schrottner@flagsmith.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #409 — base is
feat/provider-tck, so the diff here is only the report emitter. Part of open-feature/spec#424; the schema is open-feature/spec#425. This mirrors the Go implementation in go-sdk-contrib.Setting
PROVIDER_TCK_REPORT_DIRmakes each suite write its run to<dir>/<name>.json.Both documents account for all 29 scenarios. A third suite, generated by the tests, produces all four outcomes at once:
{"passed": 22, "not-declared": 5, "not-applicable": 1, "failed": 1}, again 29 of 29.Why an environment variable
It is not a
TckConfigfield, because emitting a report is a property of the run and not of the code. CI sets it, a developer running the suite locally does not, and no adopter changes a line to publish one. Unset means no report, which is the default and is not an error. Several suites in one pytest session each write their own file, so flagd's RPC and in-process resolvers would not collide — and suite names are chosen to read well in a failure message rather than to be path-safe, soflagd/rpcis sanitised toflagd-rpc.jsoninstead of writing outside the directory it was given.Why every scenario is listed
Appendix F requires that a scenario skipped for an undeclared capability is reported as skipped with the reason and never as passed. Nothing downstream can check that against a summary line, so the report records the outcome of every scenario individually — and is required to be complete, because a document that quietly dropped what it skipped would satisfy the letter of the rule and still mislead whoever read it.
pytest, unlike godog, already reports a skip honestly, so the divergence between the runner and the document shows up somewhere else here. The one scenario the Python SDK cannot satisfy is marked
xfail(strict=True)(see finding 1 in the package README), so the run exits zero. The provider still did not satisfy it, and the document saysfailedwith the reason. An expected failure is a recorded deviation, not an excused one.Four outcomes rather than two:
passedfailedxfailnot-declarednot-applicablepytest.skipThe
not-declared/not-applicablesplit is decided from the scenario's tags and the suite's declared capabilities, never from the wording of the skip message: the message is prose and the distinction is not.Scenarios are enumerated at collection and resolved into records only at the end of the session. That ordering is load-bearing. A scenario skipped by a marker never runs a fixture, so an emitter that learned of a scenario when its fixtures ran would leave it out of the document entirely — which is the exact failure mode this format exists to rule out. An autouse fixture, requested by the capability gate so it is guaranteed to run first, is only what tells the emitter which
TckConfiga module is testing.Identifying a Scenario Outline row
An entry is identified by
feature,nameandexampletogether. Every row of an outline shares one scenario name: the type-mismatch matrix inerrors.featureis eleven Examples rows under one name, and the Python run fails exactly one of them. Keyed on feature and name, those eleven entries are indistinguishable, the report cannot say which row failed, and a consumer keeps whichever it saw last — the 29 scenarios in each report collapse to 13 distinct(feature, name)pairs.Each entry from an outline now carries the row it came from, as the Examples parameters keyed by column header:
{ "feature": "errors", "name": "Requesting the wrong type returns the code default", "example": { "key": "boolean-flag", "requested": "Integer", "default": "1" }, "outcome": "failed", "reason": "expected failure: python-sdk: a boolean satisfies an Integer request …" }Values are the cell contents verbatim, as strings, because Gherkin has no types:
"1"stays"1"rather than becoming a number the table never mentioned. The field is omitted for a scenario that is not an outline, and present for a row the capability gate skipped — a skipped row is exactly as ambiguous as a failed one.Why a field and not a naming convention
The parameters are the identity, and they come from the feature file rather than from any runner. A mandated name format would put a separator, an ordering and an escaping rule into normative text that four languages have to reproduce byte for byte, and drift there is invisible until two reports silently fail to line up.
This PR is the concrete evidence. It previously worked around the missing field by appending pytest's own id for the row to the scenario name —
… [boolean-flag-Integer-1]— which is unambiguous within Python and is not what godog or the JavaScript runner produce for the same row. Three implementations of one format had already diverged on precisely this point: one emitted the bare name for all eleven rows, one appended its runner's example id, one its runner's expanded title. That workaround is removed here;nameis now the feature file's name, unqualified.Capabilities: a failed one explains itself, an untested one says nothing
Two defects in the rollup, mirroring the fix made in Go (go-sdk-contrib#944).
A failed capability was emitted as
{"state": "failed"}with no reason. The schema now requires a reason for any outcome other thanpassed, so that entry did not validate — and it appears only when a provider is actually failing, which is when the report matters most. It now reads"1 of 2 scenarios carrying @object failed; the per-scenario results say which, and why". No test caught this because every self-test suite passes, so nothing running end to end reaches that branch; the new test drives the report builder directly with synthetic records.A declared capability that no scenario carries was reported as
passed.@targetingis reserved — it is in the vocabulary and nothing tests it, because asserting that an evaluation context reached the backend needs an echo operation the control API does not have — so a provider declaring it got a green result for a claim nothing had examined. That is the vacuous pass the capability vocabulary exists to eliminate, arriving through the report instead of through the suite.Such a capability is now omitted. The suite asked no question, so it has no answer to report, and a consumer sees the tag absent rather than a pass it cannot rely on. Omitting beats inventing a fifth outcome: the four in the schema describe what the provider did, and "the suite does not test this" is a fact about the suite. Unchanged: an undeclared capability is still reported as
not-declaredwith a reason whether or not any scenario carries it, because that is a fact about the provider.Tags on an Examples block
Gherkin lets an Examples block carry its own tags, so two rows of one Scenario Outline can differ in which capability gates them. The capability gate handled that correctly — pytest-bdd attaches an Examples block's tags as marks on that block's parameter sets, and the gate reads the node's markers — but the report read a scenario's tags from the scenario, the feature and the rule, which is everywhere those tags are not.
The consequence was a misreport of exactly the kind this format exists to rule out: a row skipped because its Examples block was tagged with an undeclared capability appeared with no tags, so it was classified
not-applicablerather thannot-declared, and did not count towards the capability rollup. The row's tags are now resolved by intersecting the tags the scenario's Examples blocks declare with the markers pytest put on the node.No canonical feature file uses per-Examples tags today, so this was latent. It was found while checking a defect the Go implementation hit in the same area, where per-scenario bookkeeping keyed by scenario name let one gated row suppress the accounting for every row of its outline. Nothing here is keyed by name — the collector, the durations and the records are all keyed by pytest node id, which is unique per row — and a test covers it: with one row of a three-row outline gated, all three still appear.
Why the provider's own metadata name
provider.nameis what the provider reports throughget_metadata(), notTckConfig.name.TckConfig.nameis chosen to read well in a failure message —flagd-rpc— which makes it the configuration, and it is reported asprovider.configuration. One provider with two materially different modes therefore produces two reports that are not interchangeable, which is the property the schema'sconfigurationfield exists for. When a suite skipped every scenario and so never saw a provider, the name falls back to the suite name rather than the empty string the schema would reject.backend.controlApiis read off an optionalcontrol_apiproperty on theBackendControl, not a new protocol member: adding one would make every existing control incomplete for the sake of one string, and a control that stays quiet simply omits the field.InProcessControlreportsin-process; the plain in-memory suite's control says nothing, and its report has nocontrolApi— which is the intended behaviour, visible in the two files above.Identity, captured at build time
tck.specRevisionandtck.assetsTreecome from a generatedspec_revision.jsonthathatch_build_sync.pywrites beside the copied assets andhatch_build.pyforce-includes into the wheel and sdist. It has to be captured at build time: the submodule that knows the answer is not in the distribution, so an installed copy has nothing left to ask.The tree hash is carried as well as the commit because it identifies the assets alone — unchanged by unrelated edits elsewhere in the specification, so two runs of identical assets agree even when pinned to different commits, and checkable rather than asserted.
A build that cannot reach git warns and records
"unknown"(seven characters, the schema's minimum) rather than inventing a commit. The build never hard-fails on it.subprocess.runwith a list argv andpathlibthroughout, so the step works on Windows, Linux and macOS.Verification
Run on WSL Ubuntu, Python 3.10 (the repo's pinned version), against the schema as it stands on
feat/provider-tck-report-schema. CI does not run on this PR:build.ymltriggers only on pull requests targetingmain, and this one targetsfeat/provider-tck. Everything below is local.Package test suite.
85 passed, 9 skipped, 2 xfailed.test_report.pyaccounts for 31 of those; several run a generated one-fixture adoption in a subprocess, because the properties they check are properties of a whole pytest session.Lint and types.
ruff check,ruff format --checkandmypy(whose scope includestests) are all clean. No dependencies were added, souv.lockis untouched anduv sync --frozenis unaffected.Schema validation. Every emitted report validated with
jsonschemaas Draft 2020-12 againstspecification/assets/provider-tck/report/conformance-report.schema.json:in-memory.json{"passed": 23, "not-declared": 5, "failed": 1}controllable-in-memory.json{"passed": 24, "not-declared": 4, "failed": 1}report-fixture.json(generated by the tests){"passed": 22, "not-declared": 5, "not-applicable": 1, "failed": 1}reserved-capability.json(a suite declaring@targetingand@caching){"passed": 22, "not-declared": 6, "failed": 1}A synthetic report built directly from failing records — the only way to reach the failed-capability branch, since every suite that runs end to end passes — also validates, which is what confirms the reason is now present where the schema requires it.
The eleven rows. In both real reports,
Requesting the wrong type returns the code defaultproduces eleven entries with eleven distinctexampleobjects, matching the three Examples tables inerrors.featureexactly:{"key": "string-flag", "requested": "Boolean", "default": "false"} {"key": "string-flag", "requested": "Integer", "default": "1"} {"key": "string-flag", "requested": "Float", "default": "0.1"} {"key": "wrong-flag", "requested": "Boolean", "default": "false"} {"key": "boolean-flag", "requested": "String", "default": "fallback"} {"key": "boolean-flag", "requested": "Integer", "default": "1"} {"key": "boolean-flag", "requested": "Float", "default": "0.1"} {"key": "integer-flag", "requested": "Boolean", "default": "false"} {"key": "integer-flag", "requested": "String", "default": "fallback"} {"key": "float-flag", "requested": "Boolean", "default": "false"} {"key": "float-flag", "requested": "String", "default": "fallback"}Ten pass;
{"key": "boolean-flag", "requested": "Integer", "default": "1"}is the one that fails, and the report now says so. The examples the emitter produces are checked in the test suite against the Examples tables read out of the Gherkin by hand, rather than against pytest-bdd's parser, which is what produced them.Identity is unique. In each report,
(feature, name, example)is distinct across all 29 entries, and the outcome counts sum to 29.test_every_collected_scenario_appears_exactly_oncechecks the total against pytest's own--collect-onlycount rather than a number written down beside it, so adding a scenario to the specification cannot leave it passing while the report loses one. Ten entries per report carry noexample, which is every scenario that is not from an outline.@targetingis absent, not green. In a suite declaring@object,@targetingand@caching, the report contains@object: passedand no entry at all for@targetingor@caching. In the two real suites, which declare neither, both still appear asnot-declaredwith a reason.Tree hash is checkable.
git rev-parse dfa16586d91ca020ef1b3b82a7c972d833ff8f29:specification/assets/provider-tckin the submodule returns904aa7d5fd7a856a4f92ace24355bd1987143abc, which is what the reports carry.Packaging.
uv buildproduces a wheel containingspec_revision.jsonalongside the four feature files, the canonical flag set andcontrol-api.yaml. Building an sdist, unpacking it, deleting.gitand building a wheel from that — no submodule, no git — succeeds and carries the revision forward unchanged. The degraded path was exercised directly by pointing the sync at a directory that is not a repository: it warns twice and writes{"specRevision": "unknown", "assetsTree": ""}, which still validates.Failure to write. Verified end to end by pointing
PROVIDER_TCK_REPORT_DIRat a path under a regular file: the message appears and the session exits non-zero on an otherwise-passing run. A run that asked for a report and silently did not get one is how a publishing pipeline ends up serving a stale result forever.Things worth raising, rather than glossing
There is no overall verdict, and a mandatory failure surfaces nowhere but
scenarios. The scenario the Python SDK fails carries no capability tag — it is a mandatory scenario — so it does not roll up intocapabilities, and the top level has no field that says the run failed.in-memory.jsonshows every declared capability aspassedwhile one mandatory scenario failed. A consumer that reads onlycapabilitieswould conclude the provider is fine. Worth deciding whether the format wants a top-level verdict or whether consumers are expected to aggregatescenariosthemselves.A capability whose every scenario was skipped is still reported as
passed. The rollup counts a capability as exercised when a scenario carries its tag, and as failed only when one of those scenarios failed. A capability declared by the provider but all of whose scenarios were skipped for some other undeclared capability therefore readspassed— for instance@events, when every scenario inevents.featureis gated behind@staleor@configuration-changeand neither is declared. This matches the Go implementation, so it is left as is rather than fixed unilaterally; it is the same shape of vacuous pass the@targetingfix above addresses, and the format should probably say which of the two rules it wants.JSON key order within
exampleis not constrained. This emitter preserves the feature file's column order, which is what an object built from an ordered map gives in Python, JavaScript and Java. Go'sencoding/jsonsorts map keys. The key/value pairs agree, which is what the schema constrains and what a parsed comparison sees, but two reports of the same run are not byte-identical across languages. Worth noting if anything downstream ever hashes the serialised document.knownDeviationis not populated. The distinction it draws — "chose not to implement an optional feature" versus "has a known bug" — is exactly what anxfailed scenario is recording, and the schema now offers it at scenario level. This emitter still carries the issue reference as prose inreason. Worth wiring up once Appendix F settles how a known deviation is declared (spec#417, Q4).Not done here: CI does not upload the reports as artifacts. That belongs with whatever consumes them, and can follow.